Skip to content

Fix Issues with verifyIdToken#58

Closed
jtdLab wants to merge 25 commits intofirebase:mainfrom
jtdLab:main
Closed

Fix Issues with verifyIdToken#58
jtdLab wants to merge 25 commits intofirebase:mainfrom
jtdLab:main

Conversation

@jtdLab
Copy link
Copy Markdown

@jtdLab jtdLab commented Oct 27, 2024

Related Issues

fixes #57
fixes #56

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]).

  • I have updated the CHANGELOG.md of the relevant packages.
    Changelog files must be edited under the form:

    ## Unreleased fix/major/minor
    
    - Description of your change. (thanks to @yourGithubId)
  • If this contains new features or behavior changes,
    I have updated the documentation to match those changes.

sky-rabbit-cnc added a commit to sky-rabbit-cnc/dart_firebase_admin that referenced this pull request Nov 3, 2024
// Signature checks skipped for emulator; no need to fetch public keys.

try {
verifyJwtSignature(
Copy link
Copy Markdown
Contributor

@labrom labrom Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend maybe keeping the verifyJwtSignature function call, and modifying the function to only catch JWTExpiredException, since the handling of JWTExpiredException is the same in both verifiers.

@internal
factory TokenProvider.fromMap(Map<dynamic, dynamic> map) {
return TokenProvider(
identities: map['identities']! as Map<String, Object?>,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since behavior changed here, would you mind writing a test?

@jtdLab
Copy link
Copy Markdown
Author

jtdLab commented Nov 8, 2024

I made the changes do we need more tests for the emulator related changes?

'phone_number': 'mock-phone-number',
'picture': 'mock-picture',
'sub': 'mock-sub',
'uid': 'mock-sub',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The doc says that uid isn't actually present in the token, and this field is just a convenience that takes its value from sub. How about removing this line then?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch

@labrom
Copy link
Copy Markdown
Contributor

labrom commented Nov 8, 2024

I believe this should also fix #56, would you mind updating the description?

@jtdLab
Copy link
Copy Markdown
Author

jtdLab commented Nov 8, 2024

Done

@labrom
Copy link
Copy Markdown
Contributor

labrom commented Nov 8, 2024

Awesome, thanks!
@rrousselGit will need to review.

@irshad365
Copy link
Copy Markdown

@rrousselGit can you please review and release this fix? Thanks

@nagakuta
Copy link
Copy Markdown

nagakuta commented Jan 16, 2025

@rrousselGit or anyone else who can review and approve this pull request?

I'm eagerly awaiting this fix to be released. Thanks.

m4tbat added a commit to awtsports/dart_firebase_admin that referenced this pull request Feb 11, 2025
@rrousselGit
Copy link
Copy Markdown
Contributor

Had to move to a separate PR because of secrets. It took me way too long to realise 🤷

But LGTM, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

6 participants